Fix flaky grails-views-gson tests caused by shared static test state - #16033
Conversation
CI's flaky-test dashboard (#16030) showed ~20 flaky test methods across JsonViewHelperSpec, ExpandSpec, JsonApiSpec and JsonViewTestSpec, all sharing a common root cause: static state that leaks between specs when several of them execute in the same test JVM/fork. Two concrete leaks were found: 1. ExpandSpec declared top-level `Team`/`Player` classes with no import, which silently resolved (same package, same simple names) to the *compiled `Team`/`Player` classes already defined by JsonViewHelperSpec*. Both specs then registered those identical Class objects into their own, independently-built KeyValueMappingContext instances, so any class-keyed GORM cache populated by one spec could be observed by the other. Fixed by giving ExpandSpec its own distinct `ExpandTeam`/`ExpandPlayer` domain classes (and updating the JSON/HAL assertions, whose type names and URLs are derived from the class name). 2. `org.grails.validation.ConstraintEvalUtils` memoizes the default GORM constraints map in a single JVM-wide static field keyed by `System.identityHashCode(config)`. JsonApiSpec already worked around this for its own SuperHero fixture with hand-rolled setup()/cleanup() logic (plus reflection into Validateable's internal static field), but JsonViewHelperSpec, ExpandSpec and JsonViewTestSpec had no equivalent reset, so a stale cache entry left by whichever spec ran first in a fork could be picked up by the next. Generalized the reset by adding a `cleanup()` fixture method directly to the `JsonViewTest` trait (grails-views-gson/src/main/.../test/JsonViewTest.groovy) that clears the ConstraintEvalUtils cache after every feature, so every spec implementing the trait gets it for free. A companion `cleanupSpec()` tears down any GrailsApplication cached by org.grails.testing.GrailsUnitTest, but only once per spec class (not per-feature): GrailsUnitTest intentionally builds and reuses its GrailsApplication across an entire spec's features, and some traits (e.g. DataTest) register beans into it once per spec, so tearing it down after every feature broke DataTest-based specs (MapRenderSpec) in testing. GrailsUnitTest itself is a test-only dependency that this main-sourceSet trait cannot reference directly, so the call is made dynamically only when the implementing spec actually has it. JsonApiSpec's own setup()/cleanup() was simplified accordingly: the reflection-based Validateable static field hack is replaced with the public `SuperHero.clearConstraintsMapCache()` API (available since 7.1), and the now-redundant ConstraintEvalUtils call is dropped since the trait handles it. Verified empirically (via isolated Groovy/Spock trait-composition probes) that a class overriding a trait-provided cleanup() fails to compile under Groovy 5/Spock 2.4, which is why JsonApiSpec no longer defines cleanup() itself. Also empirically confirmed that Groovy trait static fields are *not* shared across implementing classes (contrary to the initial triage hypothesis) - the actual leak vectors are the two described above. Full :grails-views-gson:test suite (178 tests) passes repeatedly, including reruns with --rerun-tasks and varied --tests subsets/orderings covering all previously-flagged specs plus the other GrailsUnitTest+JsonViewTest specs. codeStyle and aggregateStyleViolations report zero Checkstyle/CodeNarc violations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses flakiness in :grails-views-gson:test by eliminating shared JVM/static test state leaks between specs, improving test isolation without changing production/runtime behavior.
Changes:
- Added centralized per-feature cleanup to
JsonViewTestto clear the JVM-wideConstraintEvalUtilsdefault-constraints cache. - Updated
ExpandSpecto use its own dedicated@Entitydomain classes (ExpandTeam/ExpandPlayer) to avoid accidental cross-spec class reuse and class-keyed cache leakage. - Simplified
JsonApiSpecby removing the reflection-based cache reset and usingValidateable’s publicclearConstraintsMapCache()API viaSuperHero.clearConstraintsMapCache().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy | Introduces dedicated domain classes for this spec and updates expected JSON/link values accordingly to prevent cross-spec cache leakage. |
| grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy | Removes reflection-based cache manipulation and uses the public constraints-cache clear API for the Validateable fixture. |
| grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy | Adds standardized teardown hooks to clear shared validation constraint state after each feature and optionally tear down cached GrailsApplication after the spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Before taking a look at this, I had AI take a look. Here's it's comments:
Thanks for digging into #16030 — the triage write-up is genuinely useful, and disproving the trait-static hypothesis with isolated probes rather than assuming was the right instinct. Two things I'd like to resolve before this lands.
1. The JsonViewTest changes are in src/main. The PR body says "No production code changed", but grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy ships in the grails-views-gson artifact and is the documented way applications test JSON views. Adding cleanup()/cleanupSpec() to it is a breaking API change for downstream specs (details inline — I reproduced the compile failure locally). I also believe both fixture methods are redundant with machinery that already exists in grails-testing-support-core; sources cited inline.
2. The ExpandSpec decoupling looks right, but it's applied to one of four specs with the same problem — and not to the two with the highest failure counts. Details inline on ExpandSpec.
On evidence: the counts in #16030 are worth a second look. Every flagged method within a spec has an identical count (all 8 JsonViewHelperSpec methods 20/2037, all 7 JsonApiSpec methods 20/2023, all 4 ExpandSpec methods 19/2034, JsonViewTestSpec 19/2029). Identical per-method counts across an entire spec is the signature of ~20 CI runs in which those specs failed wholesale — a fixture/setup() throw or a fork-level failure — rather than independent per-assertion flakiness. That's a different shape of bug than a cache returning stale data, and it's the strongest clue available. Could you pull the actual stack trace from one of those failing runs?
The reason I'm pushing on that: a green suite doesn't discriminate between hypotheses here. I ran :grails-views-gson:test on this branch (178/178) and then again three times with the JsonViewTest change reverted and only the ExpandSpec change kept, single JVM, -PforkEveryUnitTest=0 -PtestBisect to maximise shared state — 178/178 every time. 8.0.x passes ~99% of the time on its own, so neither result tells us whether the leak is closed.
What I'd suggest: land the ExpandSpec and JsonApiSpec changes (both are improvements on their own merits), extend the class-decoupling to the remaining specs, and drop the JsonViewTest trait change.
…d test entities jdaugherty's review on #16033 raised two issues: 1. Blocking: adding cleanup()/cleanupSpec() to the published JsonViewTest trait (grails-views-gson/src/main) breaks any downstream spec that declares its own cleanup()/cleanupSpec(), because a Groovy trait method becomes a public interface method while Spock's AST transform lowers the visibility of fixture methods it generates - the two are irreconcilable. Separately, the ConstraintEvalUtils reset this added isn't load-bearing: ConstraintEvalUtils registers its own reset as a preserved ShutdownOperations entry, so the cache is already cleared once per spec for every GrailsUnitTest spec today. Both fixture methods are removed; JsonViewTest reverts to its pre-#16033 shape. 2. The ExpandSpec fix (dedicated ExpandTeam/ExpandPlayer entities instead of an unqualified same-package reference to JsonViewHelperSpec's Team/Player) was applied to only one of several specs with the same problem, and not to the worst offenders. JsonViewHelperSpec declares Team, Player and PlayerWithAge; IncludeAssociationsSpec, HalEmbeddedSpec, IterableRenderSpec, MapRenderSpec and NullRenderingSpec all implicitly borrowed Team/Player via unqualified same-package resolution and registered the identical Class objects into their own independently-built KeyValueMappingContext/GORM mocks. HalEmbeddedSpec additionally borrowed Person from EmbeddedAssociationsSpec, and JsonApiHandleAssociationsSpec borrowed Author from JsonApiSpec. Each of these specs now gets its own spec-prefixed entity classes (IncludeAssociationsPlayer/Team, HalPlayer/Team/Person, IterableRenderPlayer/Team, MapRenderPlayer/Team, NullRenderingPlayer/Team, HandleAssociationsAuthor), so no class is ever registered into two independently-built mapping contexts. JsonViewHelperSpec, EmbeddedAssociationsSpec and JsonApiSpec keep their original classes unchanged since those are no longer borrowed by anyone else. JsonApiSpec's cleanup() (removed in the original PR when the trait started declaring one) is restored now that the trait no longer declares its own, closing the gap the reviewer noted where SuperHero's constraints cache was only reset on the way into the spec, not on the way out. Full :grails-views-gson:test (178 tests): 0 failures. codeStyle (checkstyle + CodeNarc on src/main) clean; checkstyle/codenarc on test sources are skipped project-wide, unchanged by this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16033 +/- ##
================================================
+ Coverage 0 53.6314% +53.6314%
- Complexity 0 19780 +19780
================================================
Files 0 2086 +2086
Lines 0 99630 +99630
Branches 0 17594 +17594
================================================
+ Hits 0 53433 +53433
- Misses 0 38550 +38550
- Partials 0 7647 +7647 🚀 New features to boost your workflow:
|
|
Thanks for the detailed review — all five points below are addressed in The 5 inline points:
On your stack-trace ask — still open. I couldn't produce one either. What I checked:
So three independent attempts now (yours, and this one twice) have failed to reproduce it locally, and I couldn't find a corroborating CI job failure either. Given that, I've updated the PR description to stop asserting the class-cache-leak explanation as settled and instead flag it as the working hypothesis it is. The entity-decoupling change is worth keeping on its own merits (shared |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:337
captain.id == 1Lis a no-op comparison in awhen:block (Spock only treats conditions as assertions inthen/expect). If the intent is to leave the id unset (so the expected HAL link has no id), this line should be removed or replaced with a clarifying comment; if the intent is to set the id, use assignment (=) and update the expected JSON accordingly.
def player = new HalPlayer(id: 1L, name: 'Cantona')
player.id = 1L
def captain = new HalPlayer(name: 'Keane')
captain.id == 1L
def team = new HalTeam(captain: captain, name: 'Manchester United', players: [player])
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Thanks for turning this round quickly — both asks from my last review are addressed. git diff <merge-base>..a42b487 -- grails-views-gson/src/main comes back empty, so the trait is byte-identical to base and "No production code changed" now holds, and the entity decoupling is extended to six more specs. I also appreciate how explicit the PR body is about what the evidence does and doesn't show.
Verified locally on a42b487: DO_NOT_CACHE_TESTS=1 ./gradlew :grails-views-gson:test → 178 tests, 0 failures. The branch is 314 commits behind 8.0.x, but 8.0.x has no changes under grails-views-gson since your merge base, so there are no conflicts to expect.
Two things I'd like resolved before this merges, plus one design question.
1. The decoupling pass is still incomplete, including in files this PR edits. Details inline on HalEmbeddedSpec (Address) and NullRenderingSpec (Child2). Beyond those two, TemplateInheritanceSpec still resolves Player and Circular out of JsonViewHelperSpec by the same unqualified same-package mechanism — so the commit message's claim that "JsonViewHelperSpec, EmbeddedAssociationsSpec and JsonApiSpec keep their original classes unchanged since those are no longer borrowed by anyone else" isn't accurate. There's a good reason those two are awkward to move: grails-app/views/_child{2,3,4}*.gson and circular/_circular.gson import grails.plugin.json.view.Player and grails.plugin.json.view.Circular directly, so renaming them means touching published-module templates. Please state that as the reason rather than claiming nothing borrows them. (api/PaginationSpec also imports grails.plugin.json.view.Book from JsonViewTemplateEngineSpec, but that one is an explicit import rather than a silent binding, so I'd leave it.)
2. captain.id == 1L in HalEmbeddedSpec — inline.
3. Design question: per-spec sub-packages instead of name prefixes — inline on IterableRenderSpec, where the churn is easiest to see.
Nits, none blocking:
HalEmbeddedSpecimportsgrails.gorm.annotation.Entitywhile the other new fixture blocks usegrails.persistence.Entity. Both work, and each file is internally consistent, so only worth aligning if it's cheap.- Several of the copied fixtures carry fields the borrowing spec never touches (
IncludeAssociationsTeam.captain/titles,NullRenderingTeamin its entirety). They're faithful copies of the originals, which is defensible; trimming is optional. - Two EOF nits flagged inline.
On #16030: I'd land this on its own merits, but please don't close #16030 with it, and consider retitling the PR and branch to what's actually verifiable — something like "Isolate shared test entities in grails-views-gson specs". My own runs don't discriminate between hypotheses any better than yours do; 178/178 green tells us nothing about whether the leak is closed. The identical per-method failure counts within each spec are still the strongest lead, and I'd like the issue left open pointing at that rather than treated as resolved by association.
| def p = new Person(name: 'Robert') | ||
| mappingContext.addPersistentEntities(HalPerson, Parent) | ||
| def p = new HalPerson(name: 'Robert') | ||
| p.homeAddress = new Address(postCode: '12345') |
There was a problem hiding this comment.
The Person → HalPerson rename is right, but Address is still reaching into another spec by exactly the mechanism this PR is closing: it's declared in EmbeddedAssociationsSpec (line 190) and picked up here unqualified via same-package resolution.
It matters for the same reason Person did. Address is the embedded type of both Person and HalPerson, so GormMappingConfigurationStrategy calls context.createEmbeddedEntity(Address) — see AbstractMappingContext#createEmbeddedEntity, which builds a fresh EmbeddedPersistentEntity(type, this) bound to the calling context — once for this spec and once for EmbeddedAssociationsSpec. That's the identical Class object wrapped by two independently-built mapping contexts, which is the condition the rest of the PR eliminates.
Please give this spec its own HalAddress alongside HalPerson. It's a two-line change in a file you're already editing.
| player.id = 1L | ||
| def captain = new Player(name: 'Keane') | ||
| def captain = new HalPlayer(name: 'Keane') | ||
| captain.id == 1L |
There was a problem hiding this comment.
== rather than =, so this line does nothing — Spock only treats bare conditions as assertions in then:/expect:, and this is a when: block. Pre-existing, but you're changing the lines directly above and below it, so it's free to fix here.
Worth noting the expected JSON further down asserts "href": "http://localhost:8080/halPlayer" with no id, i.e. the captain genuinely has no id and the feature is passing for the right reason. So the fix is to delete this line rather than turn it into an assignment — unless you'd rather set the id and update the expected href to /halPlayer/1.
| when: | ||
| mappingContext.addPersistentEntity(Player) | ||
| mappingContext.addPersistentEntity(NullRenderingPlayer) | ||
| def renderResult = render(templateText, [obj: new Child2()]) |
There was a problem hiding this comment.
Same pattern as the Team/Player borrowing the rest of this PR fixes: Child2 is declared in PogoDeepRenderingSpec and reached here unqualified.
Lower stakes than the entity cases — Child2 is a plain POGO, so nothing registers it into a mapping context — but it's the same silent binding, in a file you're already changing. A NullRenderingChild local to this spec closes it.
| } | ||
|
|
||
| @Entity | ||
| class IterableRenderTeam { |
There was a problem hiding this comment.
Design question on the approach as a whole, anchored here because this file shows the cost most clearly.
Prefixing the class names forces every expected-JSON string in the spec to change, and that churn is most of the +363/−219. Per-spec sub-packages would buy the same isolation for almost none of it: the JSON API type comes from PersistentEntity.decapitalizedName (DefaultJsonApiViewHelper:183) and HAL hrefs from GrailsNameUtils.getPropertyName(clazz) (TestLinkGenerator:72) — both the simple name. So grails.plugin.json.view.iterable.Player renders byte-identically to today's Player, distinct Class object and all, and every assertion in the file stays untouched.
Two reasons I lean that way:
- Rewritten assertions lose their regression value. If name derivation itself regressed, the old strings would catch it; the new ones were written to match current output.
- It's self-enforcing. Nothing in this PR stops the next spec added to
grails.plugin.json.viewfrom typingnew Player(...)and silently binding toJsonViewHelperSpecall over again. With per-spec packages that doesn't compile.
There's no template fallout to worry about: the module's only .gson fixtures live under grails-app/views and none of them are named for player or team.
This is a rework of a rename you've already done twice, so I'll leave the call to you. If you keep the prefixes, please add a line of comment above each duplicated fixture block saying why it's duplicated — otherwise someone will helpfully consolidate the seven copies back into one shared pair and reintroduce the problem.
| static constraints = { | ||
| name nullable: false | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nit: still missing the trailing newline at EOF, and this commit rewrites the tail of the file anyway.
| class HandleAssociationsAuthor { | ||
| String name | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: trailing blank line at EOF.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:372
- The expected captain self link is currently the collection URL (
/halPlayer) which matches a null id. If the captain id is meant to be set (see setup above), the expected URL should include the id to avoid asserting the wrong behavior.
"_links": {
"self": {
"href": "http://localhost:8080/halPlayer",
"hreflang": "en",
"type": "application/hal+json"
}
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:336
captain.id == 1Luses the equality operator, so it never assigns an id to the captain. This makes the test setup inconsistent with the other HAL link assertions and can produce different link output than intended.
This issue also appears on line 367 of the same file.
def player = new HalPlayer(id: 1L, name: 'Cantona')
player.id = 1L
def captain = new HalPlayer(name: 'Keane')
captain.id == 1L
def team = new HalTeam(captain: captain, name: 'Manchester United', players: [player])
|
@jdaugherty nudge |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
One new finding from this pass, and it goes to the premise rather than the diff: I went to verify the mapping-context isolation argument and it doesn't survive contact with JsonViewTest. mappingContext is a per-instance trait property, so every Spock feature already builds its own KeyValueMappingContext and re-registers the same Class objects into it. Details and the probe output are on HalEmbeddedSpec.
That doesn't make the diff wrong — replacing import grails.plugin.json.view.* inside the template strings with an explicit aliased import is a real improvement, and unqualified same-package fixture references are a real trap. It does mean the value here is naming and robustness, not flakiness, which I think should be reflected in the description before this lands.
My earlier comments are unchanged and still open.
|
|
||
| void setup() { | ||
| mappingContext.addPersistentEntities(Team, Player) | ||
| mappingContext.addPersistentEntities(HalTeam, HalPlayer) |
There was a problem hiding this comment.
This is the clearest place to raise it: I don't think the mapping-context rationale holds, and I checked rather than reasoned about it.
JsonViewTest.mappingContext is a plain trait property, not @Shared:
// grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy:67
@Autowired(required = false)
MappingContext mappingContext = {
def ctx = new KeyValueMappingContext('test')
ctx.setCanInitializeEntities(true)
return ctx
}()Groovy initialises trait properties from $init$ in the implementing class's constructor, and Spock instantiates the spec once per feature method. So every feature gets its own KeyValueMappingContext, and this setup() re-registers the same two Class objects into a fresh one each time.
I confirmed it with a throwaway spec implementing JsonViewTest — three features, setup() calling addPersistentEntities, recording System.identityHashCode of both the context and the resulting PersistentEntity:
PROBE distinct mappingContexts=3 distinct PersistentEntities=3
Three features, three independently-built contexts, three distinct PersistentEntity instances for one Class.
So "no class is registered into two independently-built mapping contexts" isn't an invariant this codebase has, and it isn't one the renames can establish. Post-rename, HalEmbeddedSpec registers HalTeam/HalPlayer into nine independently-built contexts on its own (nine features, all through this setup()); IterableRenderSpec, NullRenderingSpec and MapRenderSpec do the same explicitly, per feature, in this very diff. Whether a second spec also registers the class isn't a categorical change — it's N versus 2N of something that already happens by design.
That moves the stated root cause from "unconfirmed", where the description currently leaves it, to "contradicted by the PR's own code". I'd rather the description drop the flakiness framing than leave a hypothesis in the permanent history that the diff itself disproves.
What still stands on its own merits is narrower: the template strings previously did import grails.plugin.json.view.* into a package holding 20+ specs' fixtures, and the specs referenced Player/Team unqualified. Both are genuinely fragile and worth closing. But that's a readability and robustness argument — which is exactly what the per-spec sub-package suggestion on IterableRenderSpec buys, at a fraction of the churn.
| } | ||
|
|
||
| @Entity | ||
| class IncludeAssociationsTeam { |
There was a problem hiding this comment.
Two more borrows the sweep hasn't reached, beyond the Address and Child2 cases I flagged last time:
TemplateInheritanceSpecconstructsnew Player(...)in six places andnew Circular(...)in"test circular rendering is handled", both binding unqualified toJsonViewHelperSpec's@Entityclasses (JsonViewHelperSpec:672and:690). Those are the sameTeam/Playerfixtures the rest of this PR is decoupling, so it's the last remaining@Entitycase in the package.PaginationSpec(grails.plugin.json.view.api) importsgrails.plugin.json.view.Book, which isJsonViewTemplateEngineSpec's@LinkablePOGO — whileJsonApiSpecdeclares its ownBookinPaginationSpec's own package. Only the explicit import disambiguates the two; remove it and the reference silently flips to a different class. Lower stakes than the entity cases, but the same shape.
I'm not asking for six more renames on top of these — the opposite. Enumerating them is the argument for the sub-package approach, which would cover every case in the package mechanically and without touching a single expected-JSON string.
… names Addresses jdaugherty's design question from review: instead of renaming borrowed @entity classes with a spec-name prefix (ExpandTeam, HalPlayer, IterableRenderPlayer, ...), each spec that needs its own copy of a shared fixture now gets it in its own sub-package under grails.plugin.json.view, keeping the original simple class names (Team, Player, Person, Address, Author). JSON API `type` and HAL `href` values derive from the entity's simple class name, which a package move doesn't change, so none of the expected-JSON assertions in these specs needed to change - this is most of the earlier diff churn. It's also self-enforcing: a same-package unqualified reference can no longer silently resolve to another spec's fixture, because the classes live in different packages. Also closes two gaps jdaugherty flagged: HalEmbeddedSpec's Address is now decoupled alongside Person, and NullRenderingSpec gets its own Child POGO instead of unqualified-borrowing PogoDeepRenderingSpec's Child2. Two same-package borrows are left as-is with an explanatory comment: TemplateInheritanceSpec's Player/Circular (the published child2/child4/ circular .gson templates import grails.plugin.json.view.Player/.Circular directly, so the model must be that exact class) and PaginationSpec's explicit `import grails.plugin.json.view.Book` (disambiguates rather than silently binding, so it isn't the trap this PR closes elsewhere). Also drops the no-op `captain.id == 1L` line in HalEmbeddedSpec (a `when:` block condition, not an assertion) that jdaugherty flagged as dead code. jdaugherty separately found that JsonViewTest.mappingContext is a plain trait property, so every Spock feature already builds its own KeyValueMappingContext regardless of this PR - the mapping-context isolation this PR argued for isn't an invariant a rename can establish. The PR description is updated to reflect that this change should be evaluated as a test-hygiene/naming-safety improvement, not a confirmed fix for the flakiness in #16030. Full :grails-views-gson:test (178 tests): 0 failures. codeStyle (checkstyle + CodeNarc on src/main) clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for pushing on this — the mapping-context finding is a good catch, and I agree it changes what this PR should claim. On the premise. You're right that On the design question — done, in
Since Both remaining gaps from your last pass are closed:
Also fixed the Verified: full |
✅ All tests passed ✅🏷️ Commit: d7ee26c Learn more about TestLens at testlens.app/docs. |
Summary
~20 test methods across
JsonViewHelperSpec,JsonViewTestSpec,JsonApiSpec, andExpandSpec(module:grails-views-gson:test) show the same failure+flakinesspattern (~2% failures, ~1% flakiness per #16030). This PR started
from the hypothesis that this was caused by a class-keyed GORM cache leak across
specs that silently shared
@Entityclasses via unqualified same-packageresolution.
Status of the root-cause hypothesis: not confirmed, and probably not the cause
jdaugherty's review went to verify the mapping-context-isolation premise directly and
found it doesn't hold:
JsonViewTest.mappingContextis a plain (non-@Shared) traitproperty, and Spock instantiates a spec fresh for every feature method — so every
spec already builds a brand-new
KeyValueMappingContextper feature regardless ofthis PR, and re-registers whatever entity classes it uses into it every time. A
throwaway probe spec confirmed this empirically (3 features → 3 distinct
KeyValueMappingContexts → 3 distinctPersistentEntityinstances for the sameClass). So "no class is registered into two independently-built mapping contexts"isn't an invariant this codebase has, or one a rename can establish — whether a
second spec also registers the class is N vs 2N of something that already happens
by design.
No one has produced a stack trace from an actual failing run, despite several
independent attempts (see Testing below). The dashboard in #16030 is the only source
for the failure counts, and its underlying per-test data isn't reachable via the
GitHub API or CI artifacts.
jdaugherty's alternative theory remains the more likely explanation and is
unaddressed by this PR: every flagged method within a given spec in #16030's
dashboard shares an identical failure count (e.g. all 8
JsonViewHelperSpecmethodsat 20/2165), which looks like whole-spec/fixture-level failures (a
setup()throw ora fork-level failure) rather than independent per-assertion staleness.
What this PR actually does
Given the above, this PR is a test hygiene / naming-safety improvement, not a
flakiness fix: unqualified same-package binding to another spec's fixture classes is
a real footgun independent of whether it explains #16030, and it's worth closing on
its own merits.
Per jdaugherty's design suggestion, each spec that needs its own copy of a shared
fixture (
Team/Player/PlayerWithAge/Person/Address/Author) now gets it inits own sub-package, keeping the original simple class names, rather than
prefixing the class names as earlier revisions of this PR did:
grails.plugin.json.view.expand—ExpandSpec's ownTeam/Playergrails.plugin.json.view.include—IncludeAssociationsSpec's ownTeam/Playergrails.plugin.json.view.halembedded—HalEmbeddedSpec's ownTeam/Player/Person/Addressgrails.plugin.json.view.iterable—IterableRenderSpec's ownTeam/Playergrails.plugin.json.view.maprender—MapRenderSpec's ownTeam/Player/PlayerWithAgegrails.plugin.json.view.nullrendering—NullRenderingSpec's ownTeam/Player,plus a local
ChildPOGO replacing the previously-unaddressed unqualified borrow ofPogoDeepRenderingSpec'sChild2grails.plugin.json.view.api.handleassociations—JsonApiHandleAssociationsSpec's ownAuthorThis buys the isolation for almost none of the diff churn the earlier
prefix-rename approach caused: JSON API
typeand HALhrefvalues are derived fromthe entity's simple class name (
PersistentEntity.decapitalizedName,GrailsNameUtils.getPropertyName(clazz)), which is unchanged by moving a class to adifferent package — so none of the expected-JSON assertions in these specs needed to
change. It's also self-enforcing: a future spec added to
grails.plugin.json.viewthat types
new Player(...)no longer silently binds to another spec's fixture,because the classes live in different packages now and same-package resolution can't
reach them.
Two same-package borrows are intentionally left as-is, not moved to a
sub-package:
TemplateInheritanceSpecstill usesJsonViewHelperSpec'sPlayer/Circulardirectly (documented with a comment on the class). The published
child2/child4/circular.gsontemplates undergrails-views-gson/grails-app/viewsimportgrails.plugin.json.view.Player/.Circulardirectly, so the model passed torender()in this spec has to be that exact class — there's no fixture-onlyworkaround.
api/PaginationSpecimportsgrails.plugin.json.view.Book(
JsonViewTemplateEngineSpec's@LinkablePOGO) explicitly, rather than typing abare
Bookthat would silently resolve toJsonApiSpec's own same-packageBook.An explicit import isn't the silent-binding trap this PR closes elsewhere, so it's
left alone per review feedback.
Also unchanged from earlier revisions:
JsonViewTest(the publishedgrails-views-gsontrait) is back to its pre-PRshape — no production code changes. An earlier version of this PR added
cleanup()/cleanupSpec()directly to it, which breaks any downstream specimplementing
JsonViewTestthat declares its owncleanup()(a Groovy traitmethod must be
public; Spock's AST transform lowers the visibility of thefixture methods it generates — the two are irreconcilable).
JsonApiSpeckeeps the publicSuperHero.clearConstraintsMapCache()API (in placeof the earlier reflection hack into
Validateable's internal state) and its owncleanup().Testing
:grails-views-gson:test(178 tests): 0 failures, verified against thisrevision's sub-package restructuring.
codeStyle(checkstyle + CodeNarc onsrc/main): clean. checkstyle/CodeNarc ontest sources are skipped project-wide, unchanged by this commit.
earlier revisions of this description): all ~50 failed CI runs on
8.0.xin the30 days before this PR was opened showed no
grails-views-gsonfailure; brute-forcerepetition (single JVM/fork, 60 iterations, ~44 min) against the pre-PR base found
0 failures; jdaugherty independently got 178/178 across 4 runs, with and without
this PR's changes.
Given all of the above, this PR should be evaluated as a robustness/naming
cleanup, not a fix for the flakiness in #16030. That issue's root cause is still
open — jdaugherty's whole-spec-fixture-failure theory is the most promising lead and
would need a captured stack trace or a targeted repro to confirm.
Related: #16030